• Add targeted unit tests for gitrun, repocache, vendordir, and projlock edge cases.
• Add regression coverage for git option-injection hardening via "--" separators.
• Enforce Codecov 80% patch coverage target.

graph TD
  Repocache["internal/repocache"] -->|calls| Gitrun["internal/gitrun"] -->|executes| GitCLI["git (system)"]
  Vendordir["internal/vendordir"] -->|uses| Gitrun
  Repocache -->|reads/writes| Cache["global cache"]
  Vendordir -->|reads/writes| Cache
  Projlock["internal/projlock"] -->|lock files| Cache

Loading

The following are alternative approaches to this PR:

1. Mock git execution via injectable runner interface

• ➕ Eliminates dependency on system git binary and filesystem layout
• ➕ Makes it easy to force specific stderr/exit-code scenarios deterministically
• ➖ Requires production refactor across gitrun/repocache call sites
• ➖ Mock behavior can drift from real git semantics, reducing regression value

2. Rely on higher-level integration tests only

• ➕ Less unit-test maintenance and fewer direct internal-package tests
• ➖ Harder to cover error classification and fallback branches precisely
• ➖ Option-injection regressions can be missed without direct assertion

3. Use committed testdata repos (golden fixtures) instead of generating repos

• ➕ Stable, repeatable fixtures without scripting git in tests
• ➖ Adds binary/large fixtures to the repo and can be harder to evolve
• ➖ Less flexible for generating scenario-specific histories (tags/branches/non-tip commits)

Recommendation: Keep the current approach: tests that drive real git against ephemeral fixture repos provide strong regression coverage for fetch fallbacks and option-injection hardening with minimal production refactoring. Consider an injectable runner only if CI portability/performance becomes a recurring problem.

Bug fix (1) +2 / -2

repocache.goHarden git argument handling in repocache with "--" separators +2/-2

Harden git argument handling in repocache with "--" separators

• Adds "--" before remote/ref operands for 'git remote add' and 'git fetch' in order to prevent option-like input from being interpreted as git flags.

internal/repocache/repocache.go

Tests (6) +640 / -0

gitrun_test.goAdd direct unit tests for core gitrun helpers +308/-0

Add direct unit tests for core gitrun helpers

• Adds coverage for RemoteURL mapping, Run/RunEnv stdout+stderr and env propagation, Reachable classification, FetchSHA/FetchAll behaviors, CommitTime parsing/validation, and RemoveAll permission-fix fallback.

internal/gitrun/gitrun_test.go

projlock_test.goTest lock contention behavior and wait-hint output +63/-0

Test lock contention behavior and wait-hint output

• Adds tests ensuring Acquire blocks until context cancellation under contention and emits the >1s wait hint while still eventually succeeding.

internal/projlock/projlock_test.go

repocache_internal_test.goAdd internal tests for repocache helpers and fallback fetch paths +168/-0

Add internal tests for repocache helpers and fallback fetch paths

• Adds internal-package tests for option-injection resistance in fetchRef, isTag/pseudoVersion helpers, commitGoneErr formatting, fetchAllRefs correctness, and network error classification on unreachable remotes.

internal/repocache/repocache_internal_test.go

repocache_test.goCover EnsureCommit all-refs fallback scenario +33/-0

Cover EnsureCommit all-refs fallback scenario

• Adds a test ensuring a non-tip, non-tagged commit is resolved via the final all-refs fetch fallback (spec §5.5 step 3).

internal/repocache/repocache_test.go

linkmatch_internal_test.goTest Windows link-target normalization helper +27/-0

Test Windows link-target normalization helper

• Adds unit tests for cleanLinkTarget to ensure junction extended-length prefixes are stripped and paths are normalized before comparison.

internal/vendordir/linkmatch_internal_test.go

vendordir_test.goAdd LinkMatches validation tests for link mode +41/-0

Add LinkMatches validation tests for link mode

• Adds tests that LinkMatches returns true only for a symlink pointing to the expected store path and false for mismatches, plain files, or missing destinations.

internal/vendordir/vendordir_test.go

Documentation (1) +2 / -1

gitrun.goClarify gosec suppression contract for git invocation +2/-1

Clarify gosec suppression contract for git invocation

• Updates the gosec suppression comment to document the requirement that callers place "--" before any repo/ref/version operands when invoking git.

internal/gitrun/gitrun.go

Other (1) +5 / -0

codecov.ymlAdd Codecov patch coverage target +5/-0

Add Codecov patch coverage target

• Introduces a Codecov configuration that enforces an 80% patch coverage target for new changes.

codecov.yml

Description

Newly added Go test files under non-main packages are declaring the internal package names
(repocache and vendordir) instead of using external _test-suffixed packages (repocache_test
and vendordir_test). This violates Rule 996222’s requirement that new *_test.go files for
non-main packages use an external test package name with a _test suffix.

Code

internal/repocache/repocache_internal_test.go[3]

+package repocache

Evidence

Rule 996222 requires newly added Go test files for non-main packages to use an external test package
name ending in _test. The cited new test files show
internal/repocache/repocache_internal_test.go declares package repocache (not repocache_test)
and internal/vendordir/linkmatch_internal_test.go declares package vendordir (not
vendordir_test), directly demonstrating non-compliance with the rule.

Rule 996222: Test files must use external test packages with _test suffix
internal/repocache/repocache_internal_test.go[1-6]
internal/vendordir/linkmatch_internal_test.go[1-6]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Two newly added `*_test.go` files are using internal package declarations (`package repocache` and `package vendordir`) instead of external test packages (`repocache_test` / `vendordir_test`), which violates the external test package requirement (Rule 996222) for non-main packages.

## Issue Context
- `internal/repocache/repocache_internal_test.go` currently tests unexported helpers (e.g., `ensureBare`, `fetchRef`, `fetchAllRefs`), which is why it was written as an internal-package test.
- `internal/vendordir/linkmatch_internal_test.go` currently calls the unexported helper `cleanLinkTarget`, which is only accessible from the `vendordir` package.

## Fix Focus Areas
- internal/repocache/repocache_internal_test.go[1-10]
- internal/vendordir/linkmatch_internal_test.go[1-6]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

TestFetchRef_rejectsOptionLikeRef relies on a non-portable "touch" canary and passes
"--upload-pack=touch <path>" as a single argv element, so the canary check does not reliably
validate the intended option-injection property and may pass/fail for the wrong reason. This weakens
confidence that future regressions in argument separation will be caught.

Code

internal/repocache/repocache_internal_test.go[R32-40]

+	canary := filepath.Join(t.TempDir(), "canary")
+
+	if err := fetchRef(t.Context(), bare, "--upload-pack=touch "+canary, false); err == nil {
+		t.Fatal("fetchRef succeeded on an option-like ref, want an error")
+	}
+
+	if _, err := os.Stat(canary); err == nil {
+		t.Fatal("ref was parsed as a git option instead of a literal ref")
+	}

Evidence

The test passes an option-like ref with an embedded space as a single string, and gitrun.RunEnv
forwards args... directly to exec.CommandContext as argv entries (no shell parsing), making the
canary-file creation logic non-portable and not a direct validation of the argument-separation
property.

internal/repocache/repocache_internal_test.go[15-40]
internal/gitrun/gitrun.go[72-105]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`TestFetchRef_rejectsOptionLikeRef` aims to verify option-injection protection, but it depends on an external `touch` binary and constructs the payload as a single argv string containing a space. Because the code under test forwards args directly to `exec.CommandContext` (no shell splitting), this test does not robustly prove the security property it claims.

### Issue Context
The actual property to validate is: `fetchRef` must insert `--` so that any ref beginning with `-` is treated as a positional refspec, not as a git flag.

### Fix Focus Areas
- internal/repocache/repocache_internal_test.go[15-41]
- internal/gitrun/gitrun.go[72-105]

### Suggested fix approach
- Replace the `touch` canary strategy with a platform-independent assertion.
 - Option A (recommended): stub `git` via a temporary script/executable placed first in `PATH`.
   - The stub should record received argv to a temp file and exit non-zero.
   - Call `fetchRef(...)` and assert the recorded argv contains `fetch ... -- origin <ref>` (i.e., `--` appears before `origin`, and the ref is present as a separate argv element even when it starts with `--`).
   - Implement the stub in a cross-platform way (e.g., create a `.sh` with shebang on unix + a `.cmd` on Windows).
 - Option B: assert on git’s stderr shape in a way that does not depend on external binaries (less robust due to message variability).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

TestAcquire_printsWaitHintAfterOneSecond uses a fixed 1.2s sleep to trigger the >1s wait-hint, which
unconditionally slows the test suite and is more sensitive to timing/scheduler variability than
necessary. The same behavior can be tested deterministically by making the wait threshold
configurable in tests.

Code

internal/projlock/projlock_test.go[R127-130]

+	go func() {
+		time.Sleep(1200 * time.Millisecond)
+		release1()
+	}()

Evidence

The test includes a fixed 1.2s sleep, and the implementation’s warning threshold is hard-coded to 1
second with a polling loop, so the test cost is inherent and avoidable by making the threshold
configurable for tests.

internal/projlock/projlock_test.go[114-142]
internal/projlock/projlock.go[23-76]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The unit test `TestAcquire_printsWaitHintAfterOneSecond` introduces a hard-coded `time.Sleep(1200 * time.Millisecond)` to force the >1s warning path. This adds ~1.2s wall-clock time to every test run and can be made faster and more deterministic.

### Issue Context
`projlock.Acquire` uses constants (`warnAfter = time.Second`, `pollInterval = 50ms`) to decide when to print the wait hint.

### Fix Focus Areas
- internal/projlock/projlock.go[23-76]
- internal/projlock/projlock_test.go[114-143]

### Suggested fix approach
- Make timing parameters test-configurable while keeping production defaults.
 - e.g., change `warnAfter` / `pollInterval` from `const` to `var` (package-private), or introduce an unexported helper `acquireWithTimings(ctx, root, warn, warnAfter, pollInterval)` used by `Acquire`.
 - In the test, set `warnAfter` to a small duration (e.g., 10ms) and `pollInterval` to 1ms, then release the lock after e.g. 20–30ms.
- Optionally add a small synchronization step (channel) to ensure the contending goroutine has started waiting before releasing, so the hint path is guaranteed to be exercised.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools